Skip to content

fix(packaged): swallow EPIPE in console echo to stop main-process crash - #7446

Open
andreab67 wants to merge 2 commits into
nexu-io:mainfrom
andreab67:fix/packaged-console-echo-epipe
Open

fix(packaged): swallow EPIPE in console echo to stop main-process crash#7446
andreab67 wants to merge 2 commits into
nexu-io:mainfrom
andreab67:fix/packaged-console-echo-epipe

Conversation

@andreab67

Copy link
Copy Markdown

Reopening #6965 — the original PR was auto-closed by github-actions, and the head repo (andreab67/open-design) was momentarily in a stale deleted flag state that GitHub won't let us reopen through the API (both gh pr reopen and REST PATCH return empty 422s while head.repo.deleted is rue).

All prior approvals carry over unchanged:

  • mrcfps — APPROVED on cae687a (2026-08-17T02:18:12Z, after the head was force-pushed past the blocking review on �ec7bf3)
  • xxiaoxiong — APPROVED on cae687a (2026-08-17T01:22:22Z)

Branch tip is

Kilo and others added 2 commits August 16, 2026 19:13
Closes nexu-io#6964.

Packaged Electron builds on Windows / Linux have no controlling
terminal, so process.stdout / process.stderr are typically detached
or piped to a closed handle. The first console.info(...) a renderer
lifecycle handler fires (e.g. webContents 'did-start-loading')
raises 'Error: EPIPE: broken pipe, write' from
node:internal/streams/writable.

Because the throw happens inside Writable.write (not at the call
site), the existing console.* echo wrapper installed by
createPackagedDesktopLogger lets the error escape into the uncaught
exception path, surfacing as Electron's native 'JavaScript error in
main process' dialog and crashing the app.

Wrap each echo in a safeEcho helper that catches only the two
known-safe stream-closure codes (EPIPE, ERR_STREAM_DESTROYED);
anything else is re-thrown so real I/O failures are not silently
dropped. The structured file logger (which already swallows its own
append failures) is unaffected.

Tests: add isHarmlessStdoutError matcher coverage and two regression
tests that exercise the wrapper with simulated EPIPE / EACCES echo
failures. The narrow-match philosophy mirrors the existing
isHarmlessSocketOptionError helper for issue nexu-io#895 so a future
broadening of the filter trips a test.

The first revision of the wrapper tests stubbed console.info AFTER
constructing the logger, which meant the bare throwing stub was
hit directly and safeEcho was never exercised. Per PR nexu-io#6965 review
from mrcfps, both tests now stub the host console BEFORE the
factory so originalConsole.{info,error} captures the throwing
original, and assertions read from the real desktop log file
(logger.info is a plain function, not a vi.fn()). Both tests
verified end-to-end against the compiled createPackagedDesktopLogger
in a standalone harness.
safeEcho's try/catch only catches a synchronous throw from the echo
call. On a detached stdout/stderr pipe (no controlling terminal, the
packaged Electron case), Writable#write instead fails asynchronously:
Node schedules the stream's own 'error' event via process.nextTick
during internal destroy(), so by the time it fires, safeEcho's try
block has already returned normally and there's nothing left to catch.

Reproduced live on a packaged Windows build: the crash still occurred
with this fix in place, and the resulting stack trace was misleading —
it still showed safeEcho's frames because V8 fixes Error.stack at
construction time (inside the synchronous write attempt), not at throw
time, making it look like the try/catch was bypassed when it was never
actually reached.

Adds installStdioErrorGuard(), a second independent layer that attaches
a permanent 'error' listener directly to process.stdout/process.stderr
using the same isHarmlessStdoutError matcher (unchanged — still only
EPIPE / ERR_STREAM_DESTROYED). Regression tests emit the error
asynchronously on a fake stream rather than throwing synchronously from
a stubbed console method, since the latter only exercises the path
safeEcho already covered.
@andreab67
andreab67 requested a review from a team as a code owner August 26, 2026 12:22
@lefarcen
lefarcen requested a review from PerishCode August 26, 2026 12:26
@lefarcen lefarcen added size/M PR changes 100-300 lines risk/high High risk: apps/desktop, daemon, auth, migration, workflows, package deps type/bugfix Bug fix labels Aug 26, 2026
@lefarcen

Copy link
Copy Markdown
Contributor

Hey @andreab67 — thanks for reopening this cleanly. The packaged-startup crash path and the follow-up async stdio guard are both easy to trace from the diff.

One quick housekeeping ask before the normal review flow: could you copy the PR template details back over from #6965 here as well? Right now this PR is still missing the Why / What users will see / Surface area / Validation sections, and having that context on the live PR makes the next review pass much easier. Since this changes packaged startup behavior, I also marked it for QA validation before merge.

@lefarcen lefarcen added the needs-validation Runtime change detected; needs human or /explore agent validation. label Aug 26, 2026

@PerishCode PerishCode left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andreab67 I reviewed the packaged logging changes across both commits. The console echo now handles synchronous stream-closure errors narrowly, while the stdio error listeners cover the asynchronous EPIPE path that actually occurs with detached packaged-process pipes; unrelated errors still retain fail-fast behavior. I also verified the focused logging coverage and packaged typecheck, and confirmed the current CI failure is in an unrelated desktop PNG/Electron fixture test. Nice work carrying the fix through the asynchronous failure mode and pinning both harmless and non-harmless cases with regression coverage.

🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-validation Runtime change detected; needs human or /explore agent validation. risk/high High risk: apps/desktop, daemon, auth, migration, workflows, package deps size/M PR changes 100-300 lines type/bugfix Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants